-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implementation of Order by #537
Conversation
jenkins go |
Unit testing passed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well done
Unit testing passed. |
Unit testing passed. |
Unit testing passed. |
std::string query = "ORDER BY $-.id"; | ||
auto result = parser.parse(query); | ||
ASSERT_TRUE(result.ok()) << result.status(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can the ORDER BY
clause be used alone?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i treat ORDER BY as a sentence for now. It would not be a syntax error, but return nothing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mark a TODO to explain.
: KW_ORDER KW_BY order_factors { | ||
$$ = new OrderBySentence($3); | ||
} | ||
; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, it should be order_by_clause, not order_by_sentence.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i was discussing this with dutor and sheman before, it would be better a sentence at present, or it would made the go executor too much bloated. For further reason is that we have no optimization for execution plan for now. We should do some optimization for clause, and we'd prefer doing some go-clauses in storage layer for performance. But it is a challenge to this optimization, we'll disscuss this and put it on the agenda as soon as possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think order by
as a separate sql is weird, it should be an algorithm or a clause
return lhs.value_.date < rhs.value_.date; | ||
} | ||
break; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To date, datetime, are you comparing by string or by corresponding value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
by value, how do u think? it depends on the implementation of storage actually. Date and Datetime is not implement in storage. For now, i am doing the sort with the union type of ColumnValue, because it is comparable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although date
and datetime
is not implemented, what I really care about is how to store these two types, whether they are stored as strings or converted to timestamps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do u mean that how to store in storage?
return lhs.value_.datetime < rhs.value_.datetime; | ||
} | ||
break; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent work!
src/graph/OrderByExecutor.cpp
Outdated
void OrderByExecutor::execute() { | ||
FLOG_INFO("Executing Order By: %s", sentence_->toString().c_str()); | ||
auto comparator = [this] (cpp2::RowValue& lhs, cpp2::RowValue& rhs) { | ||
auto lhsColumns = lhs.get_columns(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are copying the columns.
IMO. You could eliminate the copying with const auto &lhsColumns = ...
, provided that lhs
and rhs
are passed by const reference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you!!!
src/graph/OrderByExecutor.cpp
Outdated
switch (lhs.type_) { | ||
case Type::bool_val: | ||
{ | ||
if (!(lhs.value_.bool_val == rhs.value_.bool_val)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason to check equality first?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As the offline discussion, the equality checking here is not necessary.
CHECK_SEMANTIC_TYPE("order", TokenType::KW_ORDER), | ||
CHECK_SEMANTIC_TYPE("ASC", TokenType::KW_ASC), | ||
CHECK_SEMANTIC_TYPE("Asc", TokenType::KW_ASC), | ||
CHECK_SEMANTIC_TYPE("asc", TokenType::KW_ASC), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
desc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
desc already exist.
{ | ||
cpp2::ExecutionResponse resp; | ||
auto &player = players_["Boris Diaw"]; | ||
// Will not do sort if field name not exist in input schema |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a bit weird. a warning to the user is better
std::string query = "ORDER BY $-.id"; | ||
auto result = parser.parse(query); | ||
ASSERT_TRUE(result.ok()) << result.status(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mark a TODO to explain.
Unit testing passed. |
Unit testing passed. |
There are conflicts with upstream. Please resolve them. |
done |
Unit testing failed. |
Unit testing passed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work.
Unit testing passed. |
* add order by sentence * Add order by executor * Add test for order by * Update order by test, parser test and scanner test. * Add InterimResult test for order by * Fix alignment problem * Delete keywords ASCEND,DESCEND * Add syntactic sugar for input_ref; Address dutor's comment. * Fix compile problem in ParserTest.
* add order by sentence * Add order by executor * Add test for order by * Update order by test, parser test and scanner test. * Add InterimResult test for order by * Fix alignment problem * Delete keywords ASCEND,DESCEND * Add syntactic sugar for input_ref; Address dutor's comment. * Fix compile problem in ParserTest.
2. remove some unused files <!-- Thanks for your contribution! In order to review PR more efficiently, please add information according to the template. --> #### What type of PR is this? - [ ] bug - [ ] feature - [x] enhancement #### What problem(s) does this PR solve? Issue(s) number: Description: 1. add comments and adjust log level for meta 2. remove some unused files - the meta/processors/kv have been never used. After talked with @critical27 , we think could remove them. - the kv relative meta client code was also removed - the MetaCommon was also unused and removed #### How do you solve it? #### Special notes for your reviewer, ex. impact of this fix, design document, etc: #### Checklist: Tests: - [ ] Unit test(positive and negative cases) - [ ] Function test - [ ] Performance test - [ ] N/A Affects: - [ ] Documentation affected (Please add the label if documentation needs to be modified.) - [ ] Incompatibility (If it breaks the compatibility, please describe it and add the label.) - [ ] If it's needed to cherry-pick (If cherry-pick to some branches is required, please label the destination version(s).) - [ ] Performance impacted: Consumes more CPU/Memory #### Release notes: Please confirm whether to be reflected in release notes and how to describe: > ex. Fixed the bug ..... Migrated from vesoft-inc#3709 Co-authored-by: pengwei.song <90180021+pengweisong@users.noreply.github.com>
No description provided.